Split codec from scheme #17922

smarterclayton · 2015-11-29T23:25:15Z

Move all serialization (codec) responsibilities out of the scheme. Separate serialization
responsibilities of codec from versioning responsibilities of codec.
Enforce that all objects have a version (add __internal and
__unversioned to represent those). Remove runtime.EmbeddedObject in favor
of pure runtime.Object -> runtime.RawExtension.

Disable the automatic encoding of runtime.Object during conversion - instead, require well behaved clients to interpret []runtime.Object themselves. This removes the need for conversion to be aware of codecs (and of objects that the conversion may not have registered).

Dramatically reduce the signature for Codec. On Decode, pass a GVK tuple that
represents the desired decoding. Return from the decode the actual origin type
that the user presented (will allow resthandler to do checking orthogonal to
behavior in codec). Serializer and Codec look the same - the Codec type is
for cases where conversion is in play, the serializer is where only raw wire
format is considered. Decode now returns enough info to make the right
decisions at both levels

Begins addressing #7111 and required to support protobuf #17854 (cannot
assume only one serialization type and 1 codec per version)

k8s-bot · 2015-11-29T23:28:55Z

GCE e2e build/test failed for commit 0be0992.

k8s-github-robot · 2015-11-29T23:31:20Z

Labelling this PR as size/XXL

deads2k · 2015-11-30T12:47:54Z

Any chance of breaking the codec method signature changes in a separate pull?

deads2k · 2015-11-30T12:49:40Z

pkg/api/latest/codec.go

+)
+
+type serializerType struct {
+	AcceptContentTypes []string


Does this extend any interfaces? Seems like I'd want this to actually be an interface.

Also, public fields on a private struct?

This is not for end users, merely to enable local code to extend it (for
now).

On Mon, Nov 30, 2015 at 7:49 AM, David Eads notifications@github.com
wrote:

In pkg/api/latest/codec.go
#17922 (comment)
:

"k8s.io/kubernetes/pkg/conversion"

"k8s.io/kubernetes/pkg/runtime"

"k8s.io/kubernetes/pkg/runtime/serializer/json"

"k8s.io/kubernetes/pkg/runtime/serializer/recognizer"
+)

+var (

Codecs CodecFactory

UniversalDeserializer runtime.Decoder

jsonSerializer = json.NewSerializer(conversion.DefaultMetaFactory, api.Scheme, runtime.ObjectTyperToTyper(api.Scheme))

yamlSerializer = json.NewYAMLSerializer(conversion.DefaultMetaFactory, api.Scheme, runtime.ObjectTyperToTyper(api.Scheme))
+)

+type serializerType struct {

AcceptContentTypes []string

Does this extend any interfaces? Seems like I'd want this to actually be
an interface.

Also, public fields on a private struct?

—
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/17922/files#r46138851.

smarterclayton · 2015-11-30T15:43:37Z

It's far too invasive - I want to get a fully working system back before I
do that. However, go ahead and make your changes, assuming that I will
pick up purely mechanical string -> GVK changes. Identify any places where
you think the assumptions are changing.

On Mon, Nov 30, 2015 at 7:48 AM, David Eads notifications@github.com
wrote:

Any chance of breaking the codec method signature changes in a separate
pull?

—
Reply to this email directly or view it on GitHub
#17922 (comment)
.

lavalamp · 2015-11-30T20:55:15Z

I have a bunch of stuff to catch up on, but I will try to look at this asap

lavalamp · 2015-12-02T00:06:50Z

pkg/api/latest/codec.go

+	"k8s.io/kubernetes/pkg/runtime/serializer/recognizer"
+)
+
+var (


Recently, every time I've written code with global variables like this, I've later regretted it and replaced with some containing object. It makes testing easier, construction more well defined, etc. Up to you though.

Yes I agree as well - I need to break the code that depends on global
codecs today, and identify where codec is an injection vs global. There
are a few places where universal decoder is all that is all that is needed,
but injection is still important.

On Dec 1, 2015, at 7:07 PM, Daniel Smith notifications@github.com wrote:

In pkg/api/latest/codec.go
#17922 (comment):

+package latest
+
+import (

"fmt"

"io"

"reflect"
+

"k8s.io/kubernetes/pkg/api"

"k8s.io/kubernetes/pkg/api/unversioned"

"k8s.io/kubernetes/pkg/conversion"

"k8s.io/kubernetes/pkg/runtime"

"k8s.io/kubernetes/pkg/runtime/serializer/json"

"k8s.io/kubernetes/pkg/runtime/serializer/recognizer"
+)
+
+var (

Recently, every time I've written code with global variables like this,
I've later regretted it and replaced with some containing object. It makes
testing easier, construction more well defined, etc. Up to you though.

—
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/17922/files#r46359926.

lavalamp · 2015-12-02T00:49:35Z

I'm sure I missed things, hopefully not big things.

The nuance of Codec and Serializer was not at all obvious to me, but I can't think of better names off the top of my head.

Is Scheme still doing any useful work?

I think the stuff in the latest package needs to be reflected in the testapi package, or extracted to some common place (I think that sort of thing was the intended purpose of Scheme?) for use by both.

smarterclayton · 2015-12-02T04:36:04Z

Agree something needs to be done with latest and testapi. As I'm walking
the changes out to packages I'm noticing the spots to change, will make
sure those get highlighted.

On Dec 1, 2015, at 7:49 PM, Daniel Smith notifications@github.com wrote:

I'm sure I missed things, hopefully not big things.

The nuance of Codec and Serializer was not at all obvious to me, but I
can't think of better names off the top of my head.

Is Scheme still doing any useful work?

I think the stuff in the latest package needs to be reflected in the
testapi package, or extracted to some common place (I think that sort of
thing was the intended purpose of Scheme?) for use by both.

—
Reply to this email directly or view it on GitHub
#17922 (comment)
.

k8s-bot · 2016-01-22T21:33:34Z

GCE e2e test build/test passed for commit 3262d8e.

lavalamp · 2016-01-22T23:30:00Z

LGTM, let's merge-- we can make incremental changes after this is in.

smarterclayton · 2016-01-23T00:30:22Z

Adding label

k8s-github-robot · 2016-01-24T20:41:14Z

@k8s-bot test this

Tests are more than 48 hours old. Re-running tests.

k8s-bot · 2016-01-24T21:14:50Z

GCE e2e test build/test passed for commit 3262d8e.

k8s-github-robot · 2016-01-25T13:56:58Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-01-25T14:30:25Z

GCE e2e test build/test passed for commit 3262d8e.

k8s-github-robot · 2016-01-25T14:30:38Z

Automatic merge from submit-queue

Auto commit by PR queue bot

smarterclayton · 2016-01-25T16:13:12Z

@k8s-oncall this is the biggest change that landed this morning, haven't ruled out it might not cause some of the more esoteric (non core e2e) tests to flake. Please ping me immediately if you see squirrelly behavior this morning.

smarterclayton · 2016-01-25T16:35:44Z

I don't think this is the cause of https://console.cloud.google.com/storage/browser/kubernetes-jenkins/logs/kubernetes-e2e-gke-ci/10448/

smarterclayton · 2016-01-25T18:02:51Z

gce-autoscaling works locally for me with gcr.io/google_containers/heapster:v0.18.4 and the latest staged source (from https://console.cloud.google.com/storage/browser/kubernetes-jenkins/logs/kubernetes-e2e-gce-autoscaling/2541/?pli=1)

googlebot added the cla: yes label Nov 29, 2015

k8s-github-robot assigned lavalamp Nov 29, 2015

k8s-github-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 29, 2015

deads2k reviewed Nov 30, 2015
View reviewed changes

smarterclayton mentioned this pull request Nov 30, 2015

Proposal: Long term evolution of runtime.Scheme / conversion.Scheme #7111

Open

lavalamp reviewed Dec 2, 2015
View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 6, 2015

smarterclayton added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 23, 2016

k8s-github-robot pushed a commit that referenced this pull request Jan 25, 2016

Merge pull request #17922 from smarterclayton/split_codec

d3b869a

Auto commit by PR queue bot

k8s-github-robot merged commit d3b869a into kubernetes:master Jan 25, 2016

smarterclayton mentioned this pull request Jan 26, 2016

Move conversion.Scheme to runtime #20127

Merged

caesarxuchao pushed a commit to caesarxuchao/kubernetes that referenced this pull request Jan 27, 2016

fix fixture for kubernetes#17922

b3bfed8

caesarxuchao pushed a commit to caesarxuchao/kubernetes that referenced this pull request Jan 27, 2016

fix fixture for kubernetes#17922

d7e1e9e

This was referenced Jan 27, 2016

crash with kubectl get -o=custom-columns #20211

Closed

Custom column printers are broken (and extract list on versioned lists is wrong) #20226

Merged

Start using ParameterCodec for clients #20252

Merged

caesarxuchao pushed a commit to caesarxuchao/kubernetes that referenced this pull request Jan 31, 2016

fix fixture for kubernetes#17922

e2d02ab

ikehz mentioned this pull request Feb 9, 2016

v1.1 e2es running against v1.2 master: 'no kind "APIVersions" is registered for version "v1"' #20824

Closed

caesarxuchao mentioned this pull request Feb 18, 2016

kubectl rolling-update nil pointer dereference #21419

Closed

abonas mentioned this pull request Feb 24, 2016

Add content-type header = application/json for create and update actions ManageIQ/kubeclient#152

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split codec from scheme #17922

Split codec from scheme #17922

smarterclayton commented Nov 29, 2015

k8s-bot commented Nov 29, 2015

k8s-github-robot commented Nov 29, 2015

deads2k commented Nov 30, 2015

deads2k Nov 30, 2015

smarterclayton Nov 30, 2015

smarterclayton commented Nov 30, 2015

lavalamp commented Nov 30, 2015

lavalamp Dec 2, 2015

smarterclayton Dec 2, 2015

lavalamp commented Dec 2, 2015

smarterclayton commented Dec 2, 2015

k8s-bot commented Jan 22, 2016

lavalamp commented Jan 22, 2016

smarterclayton commented Jan 23, 2016

k8s-github-robot commented Jan 24, 2016

k8s-bot commented Jan 24, 2016

k8s-github-robot commented Jan 25, 2016

k8s-bot commented Jan 25, 2016

k8s-github-robot commented Jan 25, 2016

smarterclayton commented Jan 25, 2016

smarterclayton commented Jan 25, 2016

smarterclayton commented Jan 25, 2016

Split codec from scheme #17922

Split codec from scheme #17922

Conversation

smarterclayton commented Nov 29, 2015

k8s-bot commented Nov 29, 2015

k8s-github-robot commented Nov 29, 2015

deads2k commented Nov 30, 2015

deads2k Nov 30, 2015

Choose a reason for hiding this comment

smarterclayton Nov 30, 2015

Choose a reason for hiding this comment

smarterclayton commented Nov 30, 2015

lavalamp commented Nov 30, 2015

lavalamp Dec 2, 2015

Choose a reason for hiding this comment

smarterclayton Dec 2, 2015

Choose a reason for hiding this comment

lavalamp commented Dec 2, 2015

smarterclayton commented Dec 2, 2015

k8s-bot commented Jan 22, 2016

lavalamp commented Jan 22, 2016

smarterclayton commented Jan 23, 2016

k8s-github-robot commented Jan 24, 2016

k8s-bot commented Jan 24, 2016

k8s-github-robot commented Jan 25, 2016

k8s-bot commented Jan 25, 2016

k8s-github-robot commented Jan 25, 2016

smarterclayton commented Jan 25, 2016

smarterclayton commented Jan 25, 2016

smarterclayton commented Jan 25, 2016